Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle DefPath hashing centrally as part of DefPathTable (+ save work during SVH calculation) #41056

Merged
merged 2 commits into from
Apr 7, 2017

Conversation

michaelwoerister
Copy link
Member

In almost all cases where we construct a DefPath, we just hash it and throw it away again immediately.
With this PR, the compiler will immediately compute and store the hash for each DefPath as it is allocated. This way we

  • can get rid of any subsequent DefPath hash caching (e.g. the DefPathHashes),
  • don't need to allocate a transient Vec for holding the DefPath (although I'm always surprised how little these small, dynamic allocations seem to hurt performance), and
  • we don't hash DefPath prefixes over and over again.

That last part is because we construct the hash for prefix::foo by hashing (hash(prefix), foo) instead of hashing every component of prefix.

The last commit of this PR is pretty neat, I think:

The SVH (Strict Version Hash) of a crate is currently computed
by hashing the ICHes (Incremental Computation Hashes) of the
crate's HIR. This is fine, expect that for incr. comp. we compute
two ICH values for each HIR item, one for the complete item and
one that just includes the item's interface. The two hashes are
are needed for dependency tracking but if we are compiling
non-incrementally and just need the ICH values for the SVH,
one of them is enough, giving us the opportunity to save some
work in this case.

r? @nikomatsakis

This PR depends on #40878 to be merged first (you can ignore the first commit for reviewing, that's just #40878).

@nikomatsakis
Copy link
Contributor

r=me after rebase

The SVH (Strict Version Hash) of a crate is currently computed
by hashing the ICHes (Incremental Computation Hashes) of the
crate's HIR. This is fine, expect that for incr. comp. we compute
two ICH values for each HIR item, one for the complete item and
one that just includes the item's interface. The two hashes are
are needed for dependency tracking but if we are compiling
non-incrementally and just need the ICH values for the SVH,
one of them is enough, giving us the opportunity to save some
work in this case.
@michaelwoerister
Copy link
Member Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Apr 7, 2017

📌 Commit bb63872 has been approved by nikomatsakis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 7, 2017
…shes, r=nikomatsakis

Handle DefPath hashing centrally as part of DefPathTable (+ save work during SVH calculation)

In almost all cases where we construct a `DefPath`, we just hash it and throw it away again immediately.
With this PR, the compiler will immediately compute and store the hash for each `DefPath` as it is allocated. This way we
+ can get rid of any subsequent `DefPath` hash caching (e.g. the `DefPathHashes`),
+ don't need to allocate a transient `Vec` for holding the `DefPath` (although I'm always surprised how little these small, dynamic allocations seem to hurt performance), and
+ we don't hash `DefPath` prefixes over and over again.

That last part is because we construct the hash for `prefix::foo` by hashing `(hash(prefix), foo)` instead of hashing every component of prefix.

The last commit of this PR is pretty neat, I think:
```
The SVH (Strict Version Hash) of a crate is currently computed
by hashing the ICHes (Incremental Computation Hashes) of the
crate's HIR. This is fine, expect that for incr. comp. we compute
two ICH values for each HIR item, one for the complete item and
one that just includes the item's interface. The two hashes are
are needed for dependency tracking but if we are compiling
non-incrementally and just need the ICH values for the SVH,
one of them is enough, giving us the opportunity to save some
work in this case.
```

r? @nikomatsakis

This PR depends on rust-lang#40878 to be merged first (you can ignore the first commit for reviewing, that's just rust-lang#40878).
bors added a commit that referenced this pull request Apr 7, 2017
Rollup of 9 pull requests

- Successful merges: #40797, #41047, #41056, #41061, #41075, #41080, #41120, #41130, #41131
- Failed merges:
@bors bors merged commit bb63872 into rust-lang:master Apr 7, 2017
@bors
Copy link
Contributor

bors commented Apr 7, 2017

⌛ Testing commit bb63872 with merge 53f4bc3...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants